Validate converted HTTP/2 headers and trailers by pavel-ptashyts · Pull Request #2275 · AsyncHttpClient/async-http-client

pavel-ptashyts · 2026-07-20T17:28:53Z

Summary

retain complete HTTP token and value validation when converting HTTP/2 response headers
apply Netty's trailer-specific restrictions to converted trailing headers
centralize pseudo-header filtering without a per-frame capturing lambda
cover malformed names and values, empty names, and prohibited trailer fields

Motivation

The original implementation assumed the HTTP/2 decoder had already performed complete header-name validation. Netty 4.2.15 validates HTTP/2 casing and pseudo-header rules, but it does not enforce the complete HTTP token syntax, and decoded values are not validated by default. Disabling name validation therefore removed the only complete check before headers reached user code.

This revision keeps Netty's normal response-header validation and uses its trailer factory for Content-Length, Transfer-Encoding, and Trailer restrictions. The shared enhanced-for copy path still avoids a capturing lambda allocation for each response and trailer frame.

Validation

JDK 11 focused Http2HandlerTest: 5 tests passed
JDK 11 ./mvnw clean verify: build, tests, Javadocs, signing, and Revapi passed

Attribution

Codex on behalf of Pavel Ptashyts

HTTP/2 decoding already validates response header names. Rechecking them while converting to HttpHeaders repeats per-character work for every response and trailer. Use a factory that disables only name validation while preserving value validation, including CR/LF rejection. Cover both properties with focused tests. Codex on behalf of Pavel Ptashyts Co-Authored-By: Codex <[email protected]>

hyperxpro · 2026-07-25T01:32:14Z

 public final class Http2Handler extends AsyncHttpClientHandler {

    private static final HttpVersion HTTP_2 = new HttpVersion("HTTP", 2, 0, true);
+    private static final HttpHeadersFactory RESPONSE_HEADERS_FACTORY =


This is the core issue. Turning off name validation here is not removing a duplicate check, it is removing the only check. Netty's HTTP/2 name validation rejects uppercase and nothing else, so everything the HTTP/1 token validator was catching now passes straight through into the headers we hand to user code.

I suggest dropping withNameValidation(false) and using DefaultHttpHeadersFactory.headersFactory() as is, or just going back to new DefaultHttpHeaders(). Keeping the factory as a hoisted static is fine and thread safe under Sharable, so that part can stay if you prefer it.

Agreed. Removed withNameValidation(false). Converted response headers now use Netty's default name and value validators.

hyperxpro · 2026-07-25T01:32:37Z

    }

+    static HttpHeaders copyHttp2Headers(Http2Headers h2Headers) {
+        // The HTTP/2 decoder validates names but not values by default. Avoid repeating the name scan while


This comment is the part I would most want fixed even after the code is sorted out. The second clause is correct, the decoder genuinely does not validate values. The first clause is not, and left in the tree it will convince the next person reading this method that the copy is safe when it is not. Comments that assert an invariant tend to outlive the reasoning behind them, so it is worth rewriting rather than deleting.

Rewrote the comment to state the actual boundary: HTTP/2 validation does not enforce complete HTTP token syntax or decoded values, so both are validated during conversion.

hyperxpro · 2026-07-25T01:33:16Z

-                trailingHeaders.add(name, entry.getValue());
-            }
-        });
+        HttpHeaders trailingHeaders = copyHttp2Headers(h2Headers);


Not introduced by this PR, but this is the natural moment to raise it since you are centralising the code. The trailers path uses the same response factory, so it never gets Netty's trailer specific restrictions. trailersFactory() additionally rejects Content-Length, Transfer-Encoding and Trailer in trailers, per RFC 9110 section 6.5.1, and I confirmed those three are rejected by that factory and accepted by the one used here.

Worth knowing that the H2 layer does not cover this either. I checked HttpHeaderValidationUtil.isConnectionHeader and it returns true for transfer-encoding, connection, te, keep-alive and upgrade, but false for content-length. So nothing anywhere currently rejects Content-Length in an HTTP/2 trailer. Happy for this to be a follow up rather than scope creep here, but note that trailersFactory().withNameValidation(false) would not help, since that flag disables the trailer restrictions too. The flag has to go first.

Applied the trailer-specific factory in this revision. Content-Length, Transfer-Encoding, and Trailer are now rejected before trailing headers reach the handler.

hyperxpro · 2026-07-25T01:33:27Z

+        // The HTTP/2 decoder validates names but not values by default. Avoid repeating the name scan while
+        // retaining the HTTP/1 value validator that rejects CR/LF and other prohibited characters.
+        HttpHeaders headers = RESPONSE_HEADERS_FACTORY.newHeaders();
+        h2Headers.forEach(entry -> {


Small aside while this loop is being touched. The lambda captures headers, so it cannot be cached and a fresh instance is allocated per response and again per trailer frame. A plain enhanced for loop over the entries removes that allocation at no cost to safety, which is a cleaner win than the one this PR is going for.

Replaced the capturing lambda with an enhanced for loop shared by response headers and trailers.

hyperxpro · 2026-07-25T01:33:39Z

+        HttpHeaders headers = RESPONSE_HEADERS_FACTORY.newHeaders();
+        h2Headers.forEach(entry -> {
+            CharSequence name = entry.getKey();
+            if (name.length() > 0 && name.charAt(0) != ':') {


Worth being aware that this guard is now the only thing stopping empty names. Previously the default validator rejected them as well. With name validation off the replacement is a not null check, so an empty name would be accepted if it ever reached the add call. Behaviour is unchanged today because this line filters them, but the backstop is gone and nothing tests this line.

Added a focused test for the empty-name guard. Empty names are skipped before insertion, while the destination validator remains the backstop for malformed non-empty names.

hyperxpro · 2026-07-25T01:34:02Z

+class Http2HandlerTest {
+
+    @Test
+    void copiesDecoderValidatedHeaderNamesWithoutRevalidation() {


This test pins the regression as intended behaviour, and the method name states the claim that does not hold. The name used here is not decoder validated. Passing false to DefaultHttp2Headers looks like it is there to construct something the decoder would have rejected, but DefaultHttp2Headers with validation on accepts this name too, because it contains no uppercase. A space is in the forbidden range in RFC 9113 section 8.2.1.

I suggest inverting it, so it asserts that a name containing CRLF is rejected. That is the property worth locking down, and it is the one that regressed. If it helps I have a reproduction that goes through the real encoder and decoder rather than constructing headers by hand, and I am happy to contribute it.

Agreed and inverted the test. It now verifies that a CRLF-containing name is rejected during conversion; the previous invalid-name acceptance claim has been removed.

hyperxpro · 2026-07-25T01:34:17Z

+    }
+
+    @Test
+    void stillValidatesHeaderValues() {


This one is good and I would keep it regardless of what happens to the rest of the PR. Value validation on this path was not covered before and it is the property that actually needs a guard, since the decoder does not check values at all.

Kept and renamed the value-validation test. It continues to verify that CRLF in a decoded header value is rejected.

Restore complete HTTP token and value validation when converting decoded HTTP/2 headers. Use Netty's trailer-specific factory so prohibited trailing fields cannot reach handlers. Replace capturing copy lambdas with enhanced loops and cover malformed names, values, empty names, and prohibited trailers. Codex on behalf of Pavel Ptashyts Co-Authored-By: Codex <[email protected]>

pavel-ptashyts · 2026-07-25T11:12:47Z

Addressed all review feedback in 0411239:

restored complete response-header name and value validation
applied trailer-specific restrictions
replaced the capturing copy lambda with an enhanced for loop
added coverage for malformed and empty names, malformed values, and prohibited trailers
corrected the PR title and description to reflect the revised scope

The focused tests and the full JDK 11 ./mvnw clean verify pass.

hyperxpro requested changes Jul 25, 2026

View reviewed changes

pavel-ptashyts changed the title ~~Avoid redundant HTTP/2 header name validation~~ Validate converted HTTP/2 headers and trailers Jul 25, 2026

Uh oh!

Conversation

pavel-ptashyts commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Motivation

Validation

Attribution

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavel-ptashyts commented Jul 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pavel-ptashyts commented Jul 20, 2026 •

edited

Loading